Skip to content

Conversation

@alexbpx
Copy link
Contributor

@alexbpx alexbpx commented Nov 21, 2018

No description provided.

headers = ctx.get('headers')
if ctx.get('block_action') is not 'r':
for item in headers.keys():
if (item.lower() is 'accept' or item.lower() is 'content-type') and headers[item] is 'application/json':

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headers[item] may be a comma separated string, where application/json is just one element, so you should check if it's contained rather than equal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


def is_json_response(self, ctx):
headers = ctx.get('headers')
if ctx.get('block_action') is not 'r':

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you call is_json_response after the check in the main flow that block action is not r, so this is a bit redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not redundant, It sets the content-type, even if the block_Action is 'r' it will get there

blocking_response = self.mustache_renderer.render(px_template.get_template(px_constants.BLOCK_TEMPLATE), blocking_props)
is_json_response = self.is_json_response(ctx)
if is_json_response:
content_type = 'application/json'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's json response, then the response shouldn't be a compiled html template

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@yaronschwimmer yaronschwimmer merged commit 4a91dc5 into dev Nov 22, 2018
@alexbpx alexbpx deleted the dev-captcha-v2 branch December 5, 2018 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants